Skip to content

rust(bug): Fix recursive/nested hdf5 imports#579

Open
Brandon-Shippy wants to merge 21 commits into
mainfrom
fix/recursive-hdf5-import
Open

rust(bug): Fix recursive/nested hdf5 imports#579
Brandon-Shippy wants to merge 21 commits into
mainfrom
fix/recursive-hdf5-import

Conversation

@Brandon-Shippy
Copy link
Copy Markdown
Contributor

@Brandon-Shippy Brandon-Shippy commented May 20, 2026

Description

  • Properly recurses through nested groups to discover all channels in a nested hdf5 file
  • Per group time pairing (1d), each dataset is paired with the timestamp in its group
  • Supports more data types to cleanly import into sift
  • Skips unknown datatypes instead of failing

Verification

  • Added unit tests for walking through groups to find channels properly.
  • All tests pass
  • Manual testing of file types with the nested structure.
  • Imported each type 1d 2d compound and properly structures in sift

Comment thread rust/crates/sift_cli/src/cmd/import/hdf5/detect_hdf5_schema.rs Outdated
Comment thread rust/crates/sift_cli/src/cmd/import/hdf5/detect_hdf5_schema.rs
Comment thread rust/crates/sift_cli/src/cmd/import/hdf5/detect_hdf5_schema.rs Outdated
Comment thread rust/crates/sift_cli/src/cmd/import/hdf5/detect_hdf5_schema.rs
Comment thread rust/crates/sift_cli/src/cmd/import/hdf5/detect_hdf5_schema.rs Outdated
Comment thread rust/crates/sift_cli/src/cmd/import/hdf5/detect_hdf5_schema.rs Outdated
Comment thread rust/crates/sift_cli/src/cmd/import/hdf5/detect_hdf5_schema.rs Outdated
Comment thread rust/crates/sift_cli/src/cmd/import/hdf5/detect_hdf5_schema.rs Outdated
Comment thread rust/crates/sift_cli/src/cmd/import/hdf5/detect_hdf5_schema.rs Outdated
@Brandon-Shippy Brandon-Shippy requested a review from solidiquis May 20, 2026 22:07
tsift
tsift previously approved these changes May 21, 2026

if group_time.is_empty() {
return Err(anyhow!(
"no time dataset found — expected one of {TIME_NAMES:?} (case-insensitive) \
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General question, how fixed are the possible time names? Wondering if longer term it could be useful to make these definable through the CLI?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assert_eq!(mapped.len(), 2);
assert_eq!(mapped[0].name, "OFF");
assert_eq!(mapped[0].key, 0);
assert!(mapped[0].is_signed);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there be a test for an unsigned enum?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

solidiquis
solidiquis previously approved these changes May 21, 2026
@Brandon-Shippy Brandon-Shippy dismissed stale reviews from solidiquis and tsift via 34b6d27 May 22, 2026 18:53
@Brandon-Shippy Brandon-Shippy requested a review from tsift May 22, 2026 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants